Conversation
…nto users/mrunalhirve/FixSDK
There was a problem hiding this comment.
Pull request overview
This PR fixes the agentId parameter handling in the Azure AI Foundry SDK integration by introducing an explicit agentInstanceId parameter to replace the implicit retrieval from turnContext.Activity.Recipient.AgenticAppId. This change improves the API design by making the agent instance identification explicit and decoupling it from the turn context.
Key Changes:
- Added
agentInstanceIdparameter to theAddToolServersToAgentAsyncmethod in both the interface and implementation - Replaced all usages of
turnContext.Activity.Recipient.AgenticAppIdwith the newagentInstanceIdparameter - Enhanced XML documentation from
<inheritdoc />to a complete parameter description
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Tooling/Extensions/AzureAIFoundry/Services/IMcpToolRegistrationService.cs |
Added agentInstanceId parameter to the AddToolServersToAgentAsync method signature with updated documentation |
src/Tooling/Extensions/AzureAIFoundry/Services/McpToolRegistrationService.cs |
Implemented the new parameter, replaced AgenticAppId extraction logic with direct parameter usage, updated documentation, and corrected logging template |
Comments suppressed due to low confidence (1)
src/Tooling/Extensions/AzureAIFoundry/Services/McpToolRegistrationService.cs:111
- The newly added agentInstanceId parameter should be validated before use. Consider adding a null/empty check similar to the agentClient validation, such as:
if (string.IsNullOrWhiteSpace(agentInstanceId)) { throw new ArgumentException("Value cannot be null or whitespace.", nameof(agentInstanceId)); }
string agentInstanceId,
UserAuthorization userAuthorization,
string authHandlerName,
ITurnContext turnContext,
string? authToken = null)
| { | ||
| // Perform the (potentially async) work in a dedicated task to keep this synchronous signature. | ||
| var (toolDefinitions, toolResources) = GetMcpToolDefinitionsAndResourcesAsync(agenticAppId, authToken ?? string.Empty, turnContext).GetAwaiter().GetResult(); | ||
| var (toolDefinitions, toolResources) = GetMcpToolDefinitionsAndResourcesAsync(agentInstanceId, authToken ?? string.Empty, turnContext).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Using GetAwaiter().GetResult() in an async method defeats the purpose of async/await and can lead to potential deadlocks. Since this method is already async, consider using await instead: var (toolDefinitions, toolResources) = await GetMcpToolDefinitionsAndResourcesAsync(agentInstanceId, authToken ?? string.Empty, turnContext).ConfigureAwait(false);
| var (toolDefinitions, toolResources) = GetMcpToolDefinitionsAndResourcesAsync(agentInstanceId, authToken ?? string.Empty, turnContext).GetAwaiter().GetResult(); | |
| var (toolDefinitions, toolResources) = await GetMcpToolDefinitionsAndResourcesAsync(agentInstanceId, authToken ?? string.Empty, turnContext).ConfigureAwait(false); |
|
|
||
| try | ||
| { | ||
| // Perform the (potentially async) work in a dedicated task to keep this synchronous signature. |
There was a problem hiding this comment.
The comment states "keep this synchronous signature" but the method signature is actually async (AddToolServersToAgentAsync). This comment appears to be outdated or incorrect. Consider removing or updating the comment to accurately reflect that this is an async method that is calling GetAwaiter().GetResult() on another async operation.
| // Perform the (potentially async) work in a dedicated task to keep this synchronous signature. |
| /// <exception cref="ArgumentNullException"></exception> | ||
| public async Task AddToolServersToAgentAsync( | ||
| PersistentAgentsClient agentClient, | ||
| string agentInstanceId, |
There was a problem hiding this comment.
You have both TurnContext and auth token here, you should use this instead of adding another parameter and letting the caller handle it:
// resolve agent identity from context or token.
string agenticAppId = Runtime.Utils.Utility.ResolveAgentIdentity(turnContext, authToken);
|
Closing this, will open new one if required |
2 participants
No description provided.